-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug] Make null values safe for IAM requests #1457
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nan-li
requested review from
emawby,
jkasten2,
jinliu9508,
shepherd-l and
jennantilla
June 26, 2024 03:13
jkasten2
requested changes
Jun 27, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we instead still make the request even if we are missing fields? Doing so allows the backend to have data on how many requests are missing what data. Only logging to the device doesn't give us this insight.
nan-li
force-pushed
the
fix/iam_nil_subscription_id_crashes
branch
from
June 27, 2024 23:03
1910593
to
5f793b8
Compare
* If subscription ID (most common) or other properties such as app ID are `nil`, the app crashes when requests for IAM impressions or clicks are created. * The app will crash with Exception `[__NSPlaceholderDictionary initWithObjects:forKeys:count:]: attempt to insert nil object from objects[1]` * Use a null-safe dictionary that will omit the entry if the value is null * We choose to send these requests so we have record of 400-ing requests
* Add test module for In App Messages * Add request tests for In App Messages including when some properties can be null * And port over 2 existing IAM request tests
nan-li
force-pushed
the
fix/iam_nil_subscription_id_crashes
branch
from
June 27, 2024 23:21
5f793b8
to
fcf9496
Compare
nan-li
changed the title
[Bug] Don't make IAM requests with no subscription ID
[Bug] Make null values safe for IAM requests
Jun 27, 2024
jkasten2
requested changes
Jun 28, 2024
iOS_SDK/OneSignalSDK/OneSignalInAppMessagesTests/OneSignalInAppMessagesTests.swift
Outdated
Show resolved
Hide resolved
jkasten2
approved these changes
Jun 28, 2024
This was referenced Jul 1, 2024
Merged
This was referenced Jul 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
One Line Summary
Make null values safe for IAM payload such as subscription ID (most common) or other data required in the payload is
nil
.Details
nil
, the app crashes when requests for IAM impressions or clicks are created.[__NSPlaceholderDictionary initWithObjects:forKeys:count:]: attempt to insert nil object from objects[1]
""
and receives a 400 response.How could this happen?
null
subscription ID, but to get IAMs, we need a subscription IDshouldShowInAppMessage
, there is an additional check for subscription ID before presenting an IAM. No IAM should be shown when subscription ID isnull
.Motivation
Fix crashes, reported in #1453 and others. The crash reports all look like they are for player ID.
Scope
Correctly handle and pass along null values.
Testing
Unit testing
Ported over 2 existing IAM-related request tests, and add a few more with null values and check it does not crash
Manual testing
Physical iPhone 13 on iOS 17.4
nil
subscription ID to theOSRequestInAppMessageViewed
andOSRequestInAppMessageClicked
I was able to recreate this scenario "naturally" with this flow:
shouldShowInAppMessage
check passesAffected code checklist
Checklist
Overview
Testing
Final pass
This change is